Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes: #15924 - Prevent API payload from allowing tagged_vlans while interface mode is set to tagged-all #17211

Merged
merged 20 commits into from
Feb 26, 2025

Conversation

DanSheps
Copy link
Member

@DanSheps DanSheps commented Aug 20, 2024

Fixes: #15924 and #17249 - Prevent API payload from allowing tagged_vlans while interface mode is set to taged-all and prevent API from allowing untagged_vlan when no 802.1q mode is set.

  • Prevent API (or any API write function) which includes untagged_vlan with interface 802.1q mode is not set
  • Prevent API (or any API write function) which includes tagged_vlans with interface mode set to tagged-all

@DanSheps DanSheps self-assigned this Aug 20, 2024
@DanSheps DanSheps added the type: bug A confirmed report of unexpected behavior in the application label Aug 20, 2024
@DanSheps DanSheps requested review from jeremystretch and removed request for jeremystretch August 21, 2024 20:17
@DanSheps DanSheps requested a review from jeremystretch August 22, 2024 13:14
@jeremystretch jeremystretch removed the type: bug A confirmed report of unexpected behavior in the application label Aug 23, 2024
@DanSheps DanSheps requested a review from jeremystretch August 25, 2024 20:44
…er model in addition to not being able to check incoming vlans under same model.
@DanSheps DanSheps requested a review from jeremystretch August 28, 2024 15:48
@DanSheps DanSheps changed the title Fixes: #15924 - Prevent API payload from allowing tagged_vlans while interface mode is set to taged-all Fixes: #15924 - Prevent API payload from allowing tagged_vlans while interface mode is set to tagged-all Aug 30, 2024
@DanSheps DanSheps changed the base branch from develop to feature August 30, 2024 02:34
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Sep 15, 2024
@DanSheps DanSheps removed the pending closure Requires immediate attention to avoid being closed for inactivity label Sep 16, 2024
@DanSheps
Copy link
Member Author

Per maintainer meeting, need to refactor logic to try and streamline the checks.

@DanSheps DanSheps marked this pull request as draft September 24, 2024 13:49
…into 15924-fix-api-interface-patch-tagged-all-mode
@DanSheps DanSheps added the status: blocked Another issue or external requirement is preventing implementation label Oct 17, 2024
@DanSheps DanSheps marked this pull request as ready for review January 21, 2025 03:01
@DanSheps DanSheps removed the status: blocked Another issue or external requirement is preventing implementation label Jan 21, 2025
@DanSheps DanSheps requested review from arthanson and removed request for jeremystretch February 24, 2025 23:11
Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanSheps I know a bit outside the scope of this issue, but we have validation here going on in 3 different spots and I think (haven't checked) if you just created an Interface model and saved it (directly in code) it would accept an invalid state for this. Would it make sense to consolidate this in the model clean or at least a common function? We should really have a code standard for this - validation in one central place. Note: Asking, if you don't think it's feasible please push back.

@DanSheps
Copy link
Member Author

We should really have a code standard for this - validation in one central place. Note: Asking, if you don't think it's feasible please push back.

You cannot check the many to many relationship on the clean in the model, which is the main driver of this change. You wouldn't set the many to many field and save on the model anyways, you would need to set() or add().

Now, we could make a Mixin which handles the validation and call that from both the API and form and at least consolidate the logic there (I did briefly think about this), but the datastructure in the API and the form is slightly different.

I would have preferred to do this on the database too but alas there is no way to perform the check.

@DanSheps DanSheps requested a review from arthanson February 26, 2025 14:19
@DanSheps
Copy link
Member Author

FYI: Tested this, the code between the form validation and the serializer validation is unfortunately too different to really support a common check (ex: form uses self.get_intial_for_field to obtain the field data from the instance, whereas api just checks the instance. Changing this to only use instance might have worked, but form as a non "None" instance for creation which means you need to check for pk in the validating function vs api which just has a None.

There are other differences too, I think, unfortunately, it makes more sense to just check on the serializer/form.

@arthanson arthanson merged commit b9b42cd into feature Feb 26, 2025
6 checks passed
@jeremystretch
Copy link
Member

After some discussion, we've decided to ship this fix in v4.2 as it doesn't quite rise to the threshold of a breaking change. I'll merge this into main as well.

@jeremystretch jeremystretch deleted the 15924-fix-api-interface-patch-tagged-all-mode branch February 28, 2025 16:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants